Improve printing of strings #22945
Improve printing of strings #22945
Conversation
som-snytt
force-pushed
the
port/sd-878-const
branch
from
98a80fa to
9f19b05
Compare
|
It would be nice to add at least a few tests. I've not found them here (https://github.com/scala/scala3/tree/main/compiler/test/dotty/tools/dotc/printing) |
|
I would do something about the code duplication before undrafting. Everything breaks if strings are broken, and on scala 2 it was also a zinc dependency. A useful test would be a scalacheck, as there are many boundary conditions. An existing test broke because it noticed whether Unicode escapes were lowercase. (I did not argue with the test.) |
som-snytt
force-pushed
the
port/sd-878-const
branch
from
9f19b05 to
e0c617f
Compare
| else if quoted then "\"" + text + "\"" | ||
| else text | ||
|
|
||
| private def escapedChar(b: StringBuilder, c: Char): Unit = |
There was a problem hiding this comment.
Can we move the functions to compiler/src/dotty/tools/dotc/util/Chars.scala to avoid duplication?
| else text | ||
|
|
||
| private def escapedChar(b: StringBuilder, c: Char): Unit = | ||
| def quadNibble(b: StringBuilder, x: Int, i: Int): Unit = |
There was a problem hiding this comment.
We don't need the parameter b here, since it is not changed.
som-snytt
force-pushed
the
port/sd-878-const
branch
from
7dac1c6 to
605e569
Compare
som-snytt
force-pushed
the
port/sd-878-const
branch
from
605e569 to
f419085
Compare
|
Inlined |
3 participants
Ports scala/scala#10897
Why was the ticket worth tackling?
It's a port of a Scala 2 support ticket, which means it is a performance improvement worth paying for. The code is not shared with Scala 2 but is similar in principle.
How I fixed it
The bottleneck is
string.flatMap(escapedChar). At the urging of the reviewer, reduced code duplication betweenPlainPrinterandSourceCodeby sticking it inutil.Chars.Why is this PR worth reviewing?
It would be more compelling with a jmh demonstration, aside from correctness. This PR is like grading homework, thankless but instructional.